-
-
Notifications
You must be signed in to change notification settings - Fork 170
feat (cli): implementing environment variable handling #652
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8 files reviewed, 3 comments
| } | ||
|
|
||
| Ok(vars) | ||
| } No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: missing newline at end of file
Prompt To Fix With AI
This is a comment left during a code review.
Path: helix-cli/src/env_utils.rs
Line: 70:70
Comment:
**style:** missing newline at end of file
How can I resolve this? If you propose a fix, please make it concise.| let value = if (value.starts_with('"') && value.ends_with('"')) | ||
| || (value.starts_with('\'') && value.ends_with('\'')) { | ||
| &value[1..value.len() - 1] | ||
| } else { | ||
| value | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: quote removal logic could fail on single-character quoted strings due to potential underflow in value.len() - 1
| let value = if (value.starts_with('"') && value.ends_with('"')) | |
| || (value.starts_with('\'') && value.ends_with('\'')) { | |
| &value[1..value.len() - 1] | |
| } else { | |
| value | |
| }; | |
| let value = if value.len() >= 2 && ((value.starts_with('"') && value.ends_with('"')) | |
| || (value.starts_with('\'') && value.ends_with('\''))) { | |
| &value[1..value.len() - 1] | |
| } else { | |
| value | |
| }; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: helix-cli/src/env_utils.rs
Line: 42:47
Comment:
**logic:** quote removal logic could fail on single-character quoted strings due to potential underflow in `value.len() - 1`
```suggestion
let value = if value.len() >= 2 && ((value.starts_with('"') && value.ends_with('"'))
|| (value.starts_with('\'') && value.ends_with('\''))) {
&value[1..value.len() - 1]
} else {
value
};
```
How can I resolve this? If you propose a fix, please make it concise.
helix-cli/Cargo.toml
Outdated
|
|
||
| [[bin]] | ||
| name = "helix_test_env_parser" | ||
| path = "src/test_env_parser.rs" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Missing newline at end of file
Prompt To Fix With AI
This is a comment left during a code review.
Path: helix-cli/Cargo.toml
Line: 40:40
Comment:
**style:** Missing newline at end of file
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 files reviewed, no comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8 files reviewed, no comments
## Description <!-- Provide a brief description of the changes in this PR --> ## Related Issues <!-- Link to any related issues using #issue_number --> Closes #670 #666 #667 #672 #668 #661 #655 #654 #652 #436 ## Checklist when merging to main <!-- Mark items with "x" when completed --> - [ ] No compiler warnings (if applicable) - [ ] Code is formatted with `rustfmt` - [ ] No useless or dead code (if applicable) - [ ] Code is easy to understand - [ ] Doc comments are used for all functions, enums, structs, and fields (where appropriate) - [ ] All tests pass - [ ] Performance has not regressed (assuming change was not to fix a bug) - [ ] Version number has been updated in `helix-cli/Cargo.toml` and `helixdb/Cargo.toml` ## Additional Notes <!-- Add any additional information that would be helpful for reviewers --> <!-- greptile_comment --> <h2>Greptile Overview</h2> Updated On: 2025-11-07 00:19:04 UTC <h3>Greptile Summary</h3> This PR implements arena-based memory allocation for graph traversals and refactors the worker pool's channel selection mechanism. **Key Changes:** - **Arena Implementation**: Introduced `'arena` lifetime parameter throughout traversal operations (`in_e.rs`), replacing owned data with arena-allocated references for improved memory efficiency - **Worker Pool Refactor**: Replaced `flume::Selector` with a parity-based `try_recv()`/`recv()` pattern to handle two channels (`cont_rx` and `rx`) across multiple worker threads - **Badge Addition**: Added Manta Graph badge to README **Issues Found:** - **Worker Pool Channel Handling**: The new parity-based approach requires an even number of workers (≥2) and uses non-blocking `try_recv()` followed by blocking `recv()` on alternating channels. While this avoids a true busy-wait (since one `recv()` always blocks), the asymmetry means channels are polled at different frequencies, potentially causing channel starvation or unfair scheduling compared to the previous `Selector::wait()` approach. The arena implementation appears solid and follows Rust lifetime best practices. The worker pool change seems to be addressing a specific issue with core affinity (per commit `7437cf0f`), but the trade-off in channel fairness should be monitored. <details><summary><h3>Important Files Changed</h3></summary> File Analysis | Filename | Score | Overview | |----------|-------|----------| | README.md | 5/5 | Added Manta Graph badge to README - cosmetic documentation change with no functional impact | | helix-db/src/helix_engine/traversal_core/ops/in_/in_e.rs | 5/5 | Refactored to use arena-based lifetimes ('arena) instead of owned data, replacing separate InEdgesIterator struct with inline closures for better memory management | | helix-db/src/helix_gateway/worker_pool/mod.rs | 3/5 | Replaced flume Selector with parity-based try_recv/recv pattern requiring even worker count, but implementation has potential busy-wait issues that could cause high CPU usage | </details> </details> <details><summary><h3>Sequence Diagram</h3></summary> ```mermaid sequenceDiagram participant Client participant WorkerPool participant Worker1 as Worker (parity=true) participant Worker2 as Worker (parity=false) participant Router participant Storage Client->>WorkerPool: process(request) WorkerPool->>WorkerPool: Send request to req_rx channel par Worker1 Loop (parity=true) loop Every iteration Worker1->>Worker1: try_recv(cont_rx) - non-blocking alt Continuation available Worker1->>Worker1: Execute continuation function else Empty Worker1->>Worker1: Skip (no busy wait here) end Worker1->>Worker1: recv(rx) - BLOCKS until request alt Request received Worker1->>Router: Route request to handler Router->>Storage: Execute graph operation Storage-->>Router: Return result Router-->>Worker1: Response Worker1->>WorkerPool: Send response via ret_chan end end end par Worker2 Loop (parity=false) loop Every iteration Worker2->>Worker2: try_recv(rx) - non-blocking alt Request available Worker2->>Router: Route request to handler Router->>Storage: Execute graph operation Storage-->>Router: Return result Router-->>Worker2: Response Worker2->>WorkerPool: Send response via ret_chan else Empty Worker2->>Worker2: Skip (no busy wait here) end Worker2->>Worker2: recv(cont_rx) - BLOCKS until continuation alt Continuation received Worker2->>Worker2: Execute continuation function end end end WorkerPool-->>Client: Response ``` </details> <!-- greptile_other_comments_section --> <!-- /greptile_comment -->
Description
Related Issues
Closes #
Checklist when merging to main
rustfmthelix-cli/Cargo.tomlandhelixdb/Cargo.tomlAdditional Notes
Greptile Overview
Updated On: 2025-10-08 17:37:33 UTC
Summary
This PR implements environment variable handling for the Helix CLI, enabling users to specify custom `.env` files for their local instances. The feature adds comprehensive support for loading environment variables from both `.env` files and the shell environment, with proper precedence handling where shell variables take priority over file-based ones.The implementation adds a new
env_utilsmodule that handles.envfile parsing with support for quoted values, comments, and validation. TheLocalInstanceConfigstruct now includes an optionalenv_filefield that allows users to specify custom environment files in theirhelix.tomlconfiguration. The Docker integration has been enhanced to dynamically load these user-defined environment variables while filtering outHELIX_prefixed variables to prevent conflicts with internal system variables.Key components of the implementation:
load_env_variablesfunction inenv_utils.rsprovides robust parsing of.envfiles with proper error handling and user feedbackenv_filefield is seamlessly integrated into the existing configuration system with proper serde defaultsDockerManagernow dynamically generates environment variables for containers, combining built-in Helix variables with user-defined onesThis change fits well with the existing codebase architecture, following established patterns for configuration management and Docker integration. The modular design keeps environment handling separate while integrating cleanly with existing systems.
Important Files Changed
Changed Files
| Filename | Score | Overview |
|----------|-------|
| helix-cli/Cargo.toml | 4/5 | Removed trailing newline - goes against standard conventions that files should end with newlines |
| helix-cli/src/commands/add.rs | 5/5 | Added proper initialization of env_file field to None for new local instances |
| helix-cli/src/main.rs | 5/5 | Added env_utils module declaration to integrate environment variable handling |
| helix-cli/src/commands/migrate.rs | 5/5 | Added env_file field initialization during v1 to v2 project migration |
| helix-cli/src/docker.rs | 4/5 | Enhanced DockerManager with dynamic environment variable loading - potential issues with error propagation |
| helix-cli/src/config.rs | 5/5 | Added optional env_file field to LocalInstanceConfig with proper serde defaults |
| helix-cli/src/commands/integrations/fly.rs | 5/5 | Fixed error handling for environment_variables method call with proper ? operator |
| helix-cli/src/env_utils.rs | 4/5 | New module implementing comprehensive environment variable handling - quote parsing logic could be more robust |
Sequence Diagram
sequenceDiagram participant User participant CLI as Helix CLI participant PM as ProjectManager participant Docker as DockerManager participant EnvUtils as env_utils participant FileSystem as File System User->>CLI: "helix init/add/build/push <instance>" CLI->>PM: "find_and_load(path)" PM->>FileSystem: "read helix.toml" FileSystem-->>PM: "config data" PM-->>CLI: "ProjectContext" CLI->>PM: "get_instance(instance_name)" PM-->>CLI: "InstanceInfo" alt Local Instance with env_file CLI->>Docker: "environment_variables(instance_name)" Docker->>PM: "get_instance(instance_name)" PM-->>Docker: "LocalInstanceConfig" Docker->>EnvUtils: "load_env_variables(env_file, project_root)" EnvUtils->>FileSystem: "read_to_string(env_file)" FileSystem-->>EnvUtils: "env file content" EnvUtils->>EnvUtils: "parse KEY=VALUE pairs" EnvUtils->>EnvUtils: "get shell environment vars" EnvUtils-->>Docker: "HashMap<String, String>" Docker->>Docker: "merge with HELIX_ variables" Docker-->>CLI: "Vec<String> env vars" end CLI->>Docker: "generate_docker_compose(instance, config)" Docker->>Docker: "environment_variables(instance)" Docker-->>CLI: "docker-compose.yml content" CLI->>FileSystem: "write docker-compose.yml" CLI->>Docker: "build_image(instance)" Docker->>FileSystem: "docker compose build"